Skip to content

Conversation

@charliecreates
Copy link
Contributor

Adds a remotely-configurable minimum supported desktop version gate (to unblock auth migrations).

Changes

  • API: add GET /api/public/desktop/version returning { minimumSupportedVersion } from DESKTOP_MINIMUM_SUPPORTED_VERSION.
  • Desktop (main): fetch config on startup (3s timeout), semver-compare vs app.getVersion(), and compute versionGate status.
  • Desktop (renderer): show a blocking Update required screen when currentVersion < minimumSupportedVersion.
  • Update CTA:
    • macOS: triggers in-app auto-update check / install.
    • Windows (and others): opens GitHub releases page.

Notes

  • Per SUPER-198, config fetch failures and invalid semver fail open (allow the user in) and log a warning.

Verification

bun run lint -- apps/desktop apps/api
bun run typecheck -- --filter=@superset/desktop --filter=@superset/api
bun run test -- --filter=@superset/desktop --filter=@superset/api

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

@charliecreates
Copy link
Contributor Author

charliecreates bot commented Dec 30, 2025

Expand this to see my work.
  • Inspected apps/desktop/package.json to confirm existing dependencies and noted that only @types/semver is present (no runtime semver), which informs feedback on whether shared/utils/semver.ts should reuse a library versus maintaining a custom comparator.
  • Reviewed apps/desktop/src/main/lib/auto-updater.ts to verify that checkForUpdates() behavior (no-op on non-macOS or in development, status transitions via AUTO_UPDATE_STATUS, and isDismissed semantics) is safe to trigger from a hard "update required" screen without surfacing conflicting toasts or dialogs.
  • Read through apps/desktop/src/main/lib/version-gate.ts (e.g., fetchMinimumSupportedVersion and getVersionGateStatus around lines 12–100) to validate timeout handling, config parsing, caching via cachedStatus, and how isSemverLt drives the isUpdateRequired flag and error paths.
  • Reviewed apps/desktop/src/shared/utils/semver.ts to confirm that parseSemver, compareSemver, and isSemverLt correctly implement semver precedence rules for numeric vs string prerelease identifiers and handle invalid inputs by returning null.
  • Checked apps/desktop/src/renderer/components/UpdateToast/useUpdateListener.tsx to ensure the trpc.autoUpdate.subscribe.useSubscription listener respects the enabled flag, properly dismisses prior toast instances via toastIdRef, and only shows custom UpdateToast UIs for terminal AUTO_UPDATE_STATUS values.
  • Reviewed apps/desktop/src/renderer/screens/update-required/index.tsx to validate how UpdateRequiredScreen consumes VersionGateStatus, subscribes to autoUpdate events, derives buttonLabel/isButtonDisabled, and sequences checkMutation vs installMutation and openUrl.mutate(RELEASES_URL) for non-auto-update platforms.
  • Verified the new API handler in apps/api/src/app/api/public/desktop/version/route.ts that exposes minimumSupportedVersion: env.DESKTOP_MINIMUM_SUPPORTED_VERSION ?? null over GET, checking that the response shape matches the expectations in fetchMinimumSupportedVersion.
  • Reviewed apps/desktop/src/lib/trpc/routers/version-gate/index.ts to confirm that the createVersionGateRouter exposes getStatus and refresh procedures correctly delegating to getVersionGateStatus with and without the { refresh: true } option, aligning with the renderer-side usage for enforcing the minimum version gate.

@github-actions
Copy link

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Fly.io Electric (Fly.io) View App
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

@charliecreates
Copy link
Contributor Author

Nice end-to-end implementation: API surface is tiny, desktop fails open on config fetch/parse issues, and the “Update required” UI is clear.

A few concrete things I’d tighten up before merging (mostly reliability / long-term maintainability):

1) Make the API response explicitly non-cacheable (remote config should actually be “remote”)
apps/api/src/app/api/public/desktop/version/route.ts currently returns a plain JSON response. Depending on how/where apps/api is deployed, it’s easy for this kind of GET endpoint to get cached somewhere (CDN/proxy/framework defaults), which would make changing DESKTOP_MINIMUM_SUPPORTED_VERSION take much longer than expected (or require redeploys).

Suggestion: set Cache-Control: no-store (and, if relevant in this Next setup, force dynamic behavior).

// apps/api/src/app/api/public/desktop/version/route.ts
export const dynamic = "force-dynamic";

export function GET() {
  return Response.json(
    { minimumSupportedVersion: env.DESKTOP_MINIMUM_SUPPORTED_VERSION ?? null },
    { headers: { "Cache-Control": "no-store" } },
  );
}

2) Semver parsing/comparison: either reuse a proven impl, or add guardrails/tests
apps/desktop/src/shared/utils/semver.ts is clean and readable, but semver edge cases are notoriously tricky.

Two specific nits if you keep the custom version:

  • localeCompare (line 68) is locale-sensitive; for semver precedence you generally want a simple deterministic lexicographic compare (e.g. aId < bId ? -1 : 1) rather than locale collation.
  • The regex accepts prerelease/build strings pretty loosely (e.g. empty prerelease identifiers). Might be fine given the controlled input, but it’s a footgun if the env var ever gets set incorrectly.

Given this is a “hard gate,” I’d strongly consider either:

  • adding a small suite of unit tests for compareSemver / isSemverLt (cover prerelease ordering rules, v prefix, build metadata ignored), or
  • using the semver package (noting you already have @types/semver in apps/desktop).

3) Version gate invalid-semver warning is a bit misleading
In apps/desktop/src/main/lib/version-gate.ts, isSemverLt() returning null could mean either currentVersion or minimumSupportedVersion is invalid, but the log says “Invalid semver in minimumSupportedVersion” (line 70). Consider logging both values (and/or tailoring configFetchError) to make future debugging less confusing.

4) Auto-update subscriptions still run even when “disabled” (minor perf + duplicate listeners)
In MainScreen, useUpdateListener({ enabled: !versionGateStatus?.isUpdateRequired }) prevents toasts, but the underlying trpc.autoUpdate.subscribe subscription is still active (useUpdateListener.tsx lines 18–62). When gated, you also subscribe again inside UpdateRequiredScreen (line 31), so you end up with two subscriptions.

Suggested refactor: split MainScreen into a small “gate” component (fetches versionGateStatus and chooses which screen to render) and a child component for the actual app that calls useUpdateListener(). That way the subscription only exists in the non-gated path, and UpdateRequiredScreen owns the only subscription in the gated path.

5) Small nits

  • apps/desktop/src/lib/trpc/routers/version-gate/index.ts: the async wrappers around getVersionGateStatus() aren’t needed (it already returns a promise). Not a big deal, but simplifies a bit.

Overall this looks close; the main “I’d really like to see this” item is the non-cacheable config endpoint, since caching would undermine the core purpose of the feature.

@Kitenite Kitenite closed this Jan 6, 2026
@Kitenite Kitenite deleted the ai-slack-c09qcu3m612-1767055290-245869 branch January 7, 2026 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants